-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Added configuration option UseStrictDomainMatching, which allows control over whether content is routed without a matching domain #19815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rol over whether content is routed without a matching domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new configuration option UseStrictDomainMatching
to control whether content is routed when there's no matching domain configured. The change addresses an issue where requests to domains that don't exist in the Umbraco configuration would still route to content under the root node, which may not be the desired behavior in multi-domain setups.
Key changes:
- Added
UseStrictDomainMatching
configuration option (defaults tofalse
for backward compatibility) - Modified
ContentFinderByUrlNew
to respect the new setting and return 404 when strict matching is enabled and no domain is found - Added comprehensive unit tests to verify the new behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
WebRoutingSettings.cs |
Adds the new UseStrictDomainMatching configuration property with documentation |
ContentFinderByUrlNew.cs |
Implements the strict domain matching logic and adds new constructor for dependency injection |
ContentFinderByUrlNewTests.cs |
Provides comprehensive unit test coverage for the new functionality |
Comments suppressed due to low confidence (1)
src/Umbraco.Core/Routing/ContentFinderByUrlNew.cs:29
- The obsolete attribute references 'Umbraco 18' but based on the current major version patterns in Umbraco, this version number may not align with the actual versioning scheme. Consider verifying the correct version number for the scheduled removal.
[Obsolete("Please use the constructor with all parameters. Scheduled for removal in Umbraco 18.")]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndyButland Thanks for adding this setting, it looks like it will work and improve multi-site installations. Just one really, really nitpicky comment I've added.
I was also unable to reproduce the other part of my issue in a clean installation. I reckon the behavior I saw was routes for an unmatched domain falling back to the first matching path in any site. During my testing I probably got confused with what domain I was using and tested with an unmatched one instead of a matched domain.
Good to hear @BenWhite27, thanks for looking over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one small thing, otherwise it tests good 🙆♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Prerequisites
Addresses: #19010
Description
The linked issue describes two concerns, only only one of which I could replicate, and which is addressed in this PR.
I've used the following setup:
With these domains:
localhost:44339
localhost:44340
I've then configured Umbraco to launch with these URLs (in
Program.cs
):Navigating to the following pages gives these results:
https://localhost:44339/sample-page/
- renders pagehttps://localhost:44339/sample-page-2/
- renders pagehttps://localhost:44340/sample-page/
- renders pagehttps://localhost:44340/sample-page-2/
- returns 404https://localhost:44341/sample-page/
- renders page (from Site A).The issue is reporting two things:
So this PR addresses the second issue. I considered it would be breaking to change this behaviour, and are situations where the current behaviour is desired (not least, when no domains are configured), so have introduced a new configuration option:
This defaults to
false
, which keeps the current behaviour. When set totrue
, the page won't be routed if a domain is not found and attached to the request provided to the content finder.With that we get the following results:
https://localhost:44339/sample-page/
- renders pagehttps://localhost:44339/sample-page-2/
- renders pagehttps://localhost:44340/sample-page/
- renders pagehttps://localhost:44340/sample-page-2/
- returns 404https://localhost:44341/sample-page/
- returns 404Testing
Review and verify what's described above.